Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

methods for setting/getting parity operating mode #1355

Merged
merged 2 commits into from
Jul 18, 2019
Merged

methods for setting/getting parity operating mode #1355

merged 2 commits into from
Jul 18, 2019

Conversation

MatthiasLohr
Copy link
Contributor

What was wrong?

Nothing. But there's always space for improvements. Since I needed functionality for getting and setting parity operating modes, I propose to add methods for that (see https://wiki.parity.io/JSONRPC-parity_set-module#parity_setmode and https://wiki.parity.io/JSONRPC-parity-module#parity_mode).

How was it fixed?

Before I searched for a cute animal picture I added two methods for easy mode control. Please check diff, it adds only 12 lines.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@kclowes
Copy link
Collaborator

kclowes commented May 17, 2019

Awesome! I just triggered a rebuild for the failing tests. And speaking of tests... do you mind adding test cases for the new methods to web3/_utils/module_testing/parity_module.py? Thanks!

@kclowes
Copy link
Collaborator

kclowes commented May 17, 2019

I just actually looked at the CI failures, and it's due to a dependency upgrade so don't worry about those. We'll get a fix in on another branch soon.

@MatthiasLohr
Copy link
Contributor Author

Nice, thanks! I'll add some tests by next week, can't make it this weekend.

@MatthiasLohr
Copy link
Contributor Author

@kclowes, is there any documentation about best practices for adding test cases?

@kclowes
Copy link
Collaborator

kclowes commented May 20, 2019

@MatthiasLohr There is not that I'm aware of, but it would be nice if there was! I'll add a new issue to add some documentation, but it would be helpful if you wanted/had time to write something up!

As far as integration testing goes, we typically just test the 'happy path', and trust that the clients have more full testing on their error paths. All the tests live in the web3/_utils/module_testing directory and then get called from the tests/integration directory. We have a testing file for each client, and each provider (ex. http, websockets, ipc). If there is a test that will fail for some reason in one client or one client/provider, then we use the @pytest.mark.xfail decorator in that permutation. For example, Parity has not implemented the eth_signTypedData function, so we've added an xfail decorator: https://github.com/ethereum/web3.py/blob/master/tests/integration/parity/common.py#L171.
There is also information about running the tests in the README. Hopefully that's helpful. Let me know if you have more questions that I can answer!

@MatthiasLohr
Copy link
Contributor Author

@kclowes, sure my tests should be added to web3/_utils/module_testing/parity_module.py? It is used as TraceModuleTest in common.py but getting/setting operational mode has nothing to do with the tracing feature...

@kclowes
Copy link
Collaborator

kclowes commented May 22, 2019

Yeah... that's a good point. The trace module tests should probably get pulled out. If you're up for it, let's keep all the tests in the same file, but add a new ParityTraceModuleTest class that will hold all of the trace module tests, and then we can put any of the parity tests that aren't in the trace module into the ParityModuleTest class. We'll need to add the ParityTraceModuleTest to module_testing/__init__.py, and then add the new class into common.py.

Or, the other path I can see is adding a ParitySetModuleTest in addition to the ParityModuleTest and the ParityTraceModuleTest with the methods you just added, and addReservedPeer. Let me know if anything doesn't make sense or if you want me to wrap this up. Thanks!

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @MatthiasLohr!

@kclowes kclowes merged commit b3c4047 into ethereum:master Jul 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants